Skip to content

Conversation

Enselic
Copy link
Member

@Enselic Enselic commented Feb 25, 2024

To make it clearer what the impact would be to stop using SIG_IGN and instead use a noop handler, like suggested here and implemented here.

Part of #97889

@rustbot
Copy link
Collaborator

rustbot commented Feb 25, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2024
@Enselic Enselic force-pushed the sigpipe-child-process branch 2 times, most recently from 9dfde0f to 527be25 Compare February 25, 2024 11:08
@Enselic Enselic mentioned this pull request Mar 9, 2024
23 tasks
@Mark-Simulacrum
Copy link
Member

Is there a reason this is a run-make test? It seems like it should be just a run-pass test like this one https://github.com/rust-lang/rust/blob/master/tests/ui/panics/abort-on-panic.rs#L1-L3 (or plenty of others with run-pass and revisions:)

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 9, 2024
@Enselic
Copy link
Member Author

Enselic commented Mar 9, 2024

@Mark-Simulacrum I don't think run-pass tests supports building the separate program that this test needs? The example you linked to runs itself as a child process, but my test can't do that, since the Rust runtime would mess with SIGPIPE in the child. (My test was originally a run-pass test until I discovered/realized that.)

@Mark-Simulacrum
Copy link
Member

Ah. I think it might be possible with auxiliary test files, but it's probably not worth the extra work then.

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 9, 2024

📌 Commit 527be25 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 9, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2024
…ark-Simulacrum

unix_sigpipe: Add test for SIGPIPE disposition in child processes

To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578).

Part of rust-lang#97889
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#121573 (unix_sigpipe: Add test for SIGPIPE disposition in child processes)
 - rust-lang#121754 ([bootstrap] Move the `split-debuginfo` setting to the per-target section)
 - rust-lang#122205 (ensure that sysroot is properly set for cross-targets)
 - rust-lang#122257 (mir-opt tests: don't run a different set on --bless; enable --keep-stage-std)
 - rust-lang#122272 (Subtree update of `rust-analyzer`)

Failed merges:

 - rust-lang#122108 (Add `target.*.runner` configuration for targets)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 11, 2024
…ark-Simulacrum

unix_sigpipe: Add test for SIGPIPE disposition in child processes

To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578).

Part of rust-lang#97889
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#121148 (Add slice::try_range)
 - rust-lang#121573 (unix_sigpipe: Add test for SIGPIPE disposition in child processes)
 - rust-lang#121633 (Win10: Use `GetSystemTimePreciseAsFileTime` directly)
 - rust-lang#121840 (Expose the Freeze trait again (unstably) and forbid implementing it manually)
 - rust-lang#121907 (skip sanity check for non-host targets in `check` builds)
 - rust-lang#122002 (std::threads: revisit stack address calculation on netbsd.)
 - rust-lang#122108 (Add `target.*.runner` configuration for targets)
 - rust-lang#122298 (RawVec::into_box: avoid unnecessary intermediate reference)

r? `@ghost`
`@rustbot` modify labels: rollup
@jhpratt
Copy link
Member

jhpratt commented Mar 11, 2024

Failed in #122327: #122327 (comment)

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 11, 2024
@Enselic Enselic force-pushed the sigpipe-child-process branch from 527be25 to 65eb51c Compare March 11, 2024 19:04
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 11, 2024
@Enselic Enselic added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 19, 2024
compiletest: Add support for `//@ aux-bin: foo.rs`

Which enables ui tests to use auxiliary binaries. See the added
self-test for an example.

This is an enabler for the test in rust-lang#121573.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2024
Rollup merge of rust-lang#122634 - Enselic:aux-bin, r=oli-obk

compiletest: Add support for `//@ aux-bin: foo.rs`

Which enables ui tests to use auxiliary binaries. See the added
self-test for an example.

This is an enabler for the test in rust-lang#121573.
@bors
Copy link
Collaborator

bors commented Mar 19, 2024

☔ The latest upstream changes (presumably #122735) made this pull request unmergeable. Please resolve the merge conflicts.

@Enselic Enselic force-pushed the sigpipe-child-process branch from 0dc26de to 3095794 Compare March 20, 2024 05:21
@Enselic
Copy link
Member Author

Enselic commented Mar 20, 2024

@Mark-Simulacrum After rebasing on #122634 the only commit left in this PR is the test itself (which is now a regular run-pass ui test) which you more or less already approved, so would be great if you could take another look.

@Enselic Enselic added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2024
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me if we really need the extra ignore

@@ -0,0 +1,56 @@
//@ revisions: default sig_dfl sig_ign inherit
//@ ignore-cross-compile because we run the compiled code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't present in most run-pass tests, so what makes this one special?

Copy link
Member Author

@Enselic Enselic Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it is not run-pass that is the problem here, because the following run-pass and aux-build test passes in cross-compilation context:

src/ci/docker/run.sh --dev armhf-gnu
../x test --target arm-unknown-linux-gnueabihf tests/ui/cross-crate/reexported-static-methods-cross-crate.rs

Instead, it is aux-bin that does not support cross compilation yet. For example, remote-test-client does not yet know how to upload auxiliary binaries to the target device. That it something that would be nice to solve, but it does not block this PR. I updated the motivation for the ignore.

I would be happy to @bors r=Mark-Simulacrum the latest commit but you'll need to @bors delegate=Enselic first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 24, 2024
Enselic added 2 commits March 25, 2024 06:23
For robustness, also test the disposition in our own process even if
other tests in `tests/ui/attributes/unix_sigpipe` already covers it.
@Enselic Enselic force-pushed the sigpipe-child-process branch from 3095794 to 71eb763 Compare March 25, 2024 05:26
@Enselic Enselic added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 25, 2024
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Mar 30, 2024

📌 Commit 71eb763 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 30, 2024
…ark-Simulacrum

unix_sigpipe: Add test for SIGPIPE disposition in child processes

To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578).

Part of rust-lang#97889
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 30, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#121573 (unix_sigpipe: Add test for SIGPIPE disposition in child processes)
 - rust-lang#123170 (Replace regions in const canonical vars' types with `'static` in next-solver canonicalizer)
 - rust-lang#123200 (KCFI: Require -C panic=abort)
 - rust-lang#123201 (Improve wording in std::any explanation)
 - rust-lang#123224 (compiletest: print reason for failing to read tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5977d63 into rust-lang:master Mar 30, 2024
@rustbot rustbot added this to the 1.79.0 milestone Mar 30, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 30, 2024
Rollup merge of rust-lang#121573 - Enselic:sigpipe-child-process, r=Mark-Simulacrum

unix_sigpipe: Add test for SIGPIPE disposition in child processes

To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578).

Part of rust-lang#97889
@Enselic Enselic deleted the sigpipe-child-process branch March 30, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants